websocket, internal/testing: support multiconn WS helpers by thrasher- · Pull Request #2205 · thrasher-corp/gocryptotrader

thrasher- · 2026-03-10T03:50:34Z

PR Description

Fix internal testing websocket helpers so they work with multi-connection managers.

This addresses issue #2151 by updating the shared websocket test helpers instead of relying on per-exchange workarounds.

What changed

added SetSubscriptionsNotRequired() support for multi-connection managers
added SetAllConnectionURLs() so test helpers can redirect all managed websocket connections to a single mock server
updated internal/testing/exchange.SetupWs to use the multi-connection-safe helper path
updated internal/testing/exchange.MockWsInstance to use the multi-connection-safe helper path
added direct unit coverage for the new manager helper methods
added helper-level regression coverage for:
- MockWsInstance with a real multi-connection exchange
- SetupWs with a synthetic multi-connection setup
added a focused Poloniex proof-of-concept test showing the shared helper works on the exchange that originally triggered the issue

Why

SetupWs and MockWsInstance previously assumed single-connection websocket behaviour. That broke down once exchanges used UseMultiConnectionManagement, because empty generated subscriptions prevented connections from being established and test helpers had no clean way to mark subscriptions as not required.

This change keeps the fix in shared test infrastructure so exchanges do not need ad-hoc helper logic.

Type of change

Bug fix
Tests

How has this been tested

go test ./exchange/websocket -count=1
go test ./internal/testing/exchange -count=1 -v
go test ./exchanges/bybit -count=1
go test ./exchanges/poloniex -count=1
golangci-lint run ./exchange/websocket ./internal/testing/exchange ./exchanges/poloniex
codespell
gofumpt

Closes #2151

Copilot

Pull request overview

This PR fixes shared websocket test helpers (SetupWs and MockWsInstance) to work with exchanges that use multi-connection websocket management. Previously, these helpers assumed single-connection behavior, which broke for exchanges using UseMultiConnectionManagement.

Changes:

Added SetSubscriptionsNotRequired() and SetAllConnectionURLs() methods to the websocket Manager to support both single and multi-connection modes.
Updated MockWsInstance and SetupWs test helpers to use the new multi-connection-safe methods.
Added unit tests for the new manager methods and integration-level regression tests including a Poloniex proof-of-concept.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
exchange/websocket/manager.go	Added `SetSubscriptionsNotRequired()` and `SetAllConnectionURLs()` methods to `Manager`
internal/testing/exchange/exchange.go	Updated `MockWsInstance` and `SetupWs` to use the new multi-connection-safe helpers
exchange/websocket/manager_test.go	Added unit tests for `SetSubscriptionsNotRequired` and `SetAllConnectionURLs`
internal/testing/exchange/exchange_test.go	Added regression tests for multi-connection support in `MockWsInstance` and `SetupWs`
exchanges/poloniex/poloniex_websocket_helper_test.go	Added Poloniex-specific proof-of-concept test for multi-connection `SetupWs`

Copilot · 2026-03-10T03:52:30Z

exchange/websocket/manager.go

+	for _, ws := range m.connectionManager {
+		if ws.setup == nil {
+			ws.setup = &ConnectionSetup{}
+		}
+		ws.setup.SubscriptionsNotRequired = true
+	}


Creating a new ConnectionSetup{} for a nil ws.setup silently masks a programming error — a websocket entry in connectionManager should always have been initialized via SetupNewConnection. This guard could hide bugs where the connection manager is in an inconsistent state. Consider returning an error or logging a warning instead of silently initializing an empty setup.

Jarvis: I’m keeping this guard as-is. In normal flow multi-connection entries are created via SetupNewConnection with a non-nil setup, so this is a cheap defensive safeguard rather than a primary code path, and I’d rather keep the helper resilient here than widen it into a broader error-return change.

I think its better to at least log a warning as this should be set up correctly

Jarvis: Done in thrasher-@ba823c2 — when this helper encounters a nil connection setup it now logs a warning before creating an empty setup, so the defensive guard stays visible instead of silent.

Copilot · 2026-03-10T03:52:30Z

exchange/websocket/manager.go

+	for _, ws := range m.connectionManager {
+		if ws.setup == nil {
+			ws.setup = &ConnectionSetup{}
+		}
+		ws.setup.URL = u
+		for _, conn := range ws.connections {
+			conn.SetURL(u)
+		}
+	}


Same concern as in SetSubscriptionsNotRequired: silently initializing a nil ws.setup with &ConnectionSetup{} could mask an invalid state. Both methods should handle this consistently — consider extracting the nil-guard into a shared helper or removing it in favor of treating a nil setup as an error.

Jarvis: Same reasoning here — I’m keeping the guard. The nil setup case should not occur in normal multi-connection setup flow, and the defensive check keeps the two helper methods consistent without changing behaviour for valid managers.

I think logging is fine here.

Jarvis: Done in thrasher-@ba823c2 — same treatment here: a nil setup now logs a warning before the empty setup is created.

Copilot · 2026-03-10T03:52:31Z

internal/testing/exchange/exchange.go

 	// Exchanges which don't support subscription conf; Can be removed when all exchanges support sub conf
 	b.Websocket.GenerateSubs = func() (subscription.List, error) { return subscription.List{}, nil }


In MockWsInstance, GenerateSubs is still explicitly overridden on line 126, even though SetSubscriptionsNotRequired() on line 127 already sets GenerateSubs to return an empty list for single-connection managers. This makes the two lines redundant for the single-connection path. Consider removing the explicit GenerateSubs override or adding a comment explaining why both are needed (e.g., if SetSubscriptionsNotRequired doesn't override GenerateSubs for multi-connection managers and the old override is kept as a safety net for the single-connection case).

Suggested change

// Exchanges which don't support subscription conf; Can be removed when all exchanges support sub conf

b.Websocket.GenerateSubs = func() (subscription.List, error) { return subscription.List{}, nil }

// Ensure no default subscriptions are required for tests, including exchanges without subscription configuration support.

Jarvis: Done in thrasher-@669594f — removed the redundant GenerateSubs override so MockWsInstance now relies on SetSubscriptionsNotRequired() as the single source of that behaviour.

gemini-code-assist

Code Review

This pull request effectively addresses the issue of supporting multi-connection websocket helpers for testing. The introduction of SetSubscriptionsNotRequired and SetAllConnectionURLs in the websocket manager provides a clean way to configure test environments, and the updates to MockWsInstance and SetupWs correctly leverage these new helpers. The testing strategy is thorough, with new unit tests, regression tests for multi-connection exchanges, and a proof-of-concept test for Poloniex. I have one minor suggestion to improve code clarity in a test helper, as detailed in the specific comment.

gemini-code-assist · 2026-03-10T03:53:09Z

internal/testing/exchange/exchange.go

 	b.Websocket.GenerateSubs = func() (subscription.List, error) { return subscription.List{}, nil }
+	b.Websocket.SetSubscriptionsNotRequired()


The call to b.Websocket.SetSubscriptionsNotRequired() on line 127 makes the assignment on line 126 redundant. The SetSubscriptionsNotRequired function already handles setting GenerateSubs for single-connection managers. To improve maintainability and avoid confusion, you can remove this redundant line. The preceding comment on line 125 also appears to be obsolete with this change.

Suggested change

b.Websocket.GenerateSubs = func() (subscription.List, error) { return subscription.List{}, nil }

b.Websocket.SetSubscriptionsNotRequired()

b.Websocket.SetSubscriptionsNotRequired()

Jarvis: Done in thrasher-@669594f — removed the redundant GenerateSubs assignment and the now-obsolete extra comment so this helper only uses SetSubscriptionsNotRequired() for that behaviour.

shazbert

🚀

shazbert · 2026-03-10T22:21:56Z

exchange/websocket/manager.go

+	for _, ws := range m.connectionManager {
+		if ws.setup == nil {
+			ws.setup = &ConnectionSetup{}
+		}
+		ws.setup.SubscriptionsNotRequired = true
+	}


I think its better to at least log a warning as this should be set up correctly

exchange/websocket/manager.go

shazbert · 2026-03-10T22:25:00Z

exchange/websocket/manager.go

+	for _, ws := range m.connectionManager {
+		if ws.setup == nil {
+			ws.setup = &ConnectionSetup{}
+		}
+		ws.setup.URL = u
+		for _, conn := range ws.connections {
+			conn.SetURL(u)
+		}
+	}


I think logging is fine here.

exchange/websocket/manager_test.go

shazbert

Thanks!

gloriousCode · 2026-03-11T05:00:32Z

exchange/websocket/manager.go

+// It is intended as a pre-connect test-mode mutation; tests that later need
+// the manager's normal default-subscription bootstrap should use a fresh
+// exchange/manager instance.
+func (m *Manager) SetSubscriptionsNotRequired() {


I think these testonly functions should be part of their own file. Its wasting space in an already convoluted package/file.
While splitting it can steer it to begin even more convoluted, at least by virtue of the filename users will know not to call them under normal operations. The comment is weak with "This is primarily used by test helpers", it is only for testing purposes that these exist, so I think you should make that comment stronger

I can still be persuaded to not split it out into its own file, but please at least improve the comments

Jarvis: Done in thrasher-@9332623 — I moved the test-only hooks out of manager.go into exchange/websocket/manager_testhelpers.go and tightened the comments to make it explicit they’re only for cross-package test harnesses / pre-connect test setup.

You're alright Jarvis, no need to unplug you yet

thrasher- · 2026-03-11T05:58:01Z

@codex please review

chatgpt-codex-connector · 2026-03-11T06:04:01Z

Codex Review: Didn't find any major issues. You're on a roll.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

gloriousCode

tACK

shazbert

ch ACK!

websocket, internal/testing: support multiconn WS helpers

cd4f8f4

Copilot AI review requested due to automatic review settings March 10, 2026 03:50

thrasher- added jarvis Automated updates by Jarvis test(s) fix This is to denote the PR is fixing a build test issue labels Mar 10, 2026

Copilot started reviewing on behalf of thrasher- March 10, 2026 03:50 View session

thrasher- requested review from cranktakular, gloriousCode, samuael and shazbert March 10, 2026 03:51

thrasher- self-assigned this Mar 10, 2026

thrasher- added the review me This pull request is ready for review label Mar 10, 2026

Copilot AI reviewed Mar 10, 2026

View reviewed changes

gemini-code-assist bot reviewed Mar 10, 2026

View reviewed changes

internal/testing: remove redundant subs override

669594f

shazbert requested changes Mar 10, 2026

View reviewed changes

websocket: harden multiconn test helpers

ba823c2

shazbert approved these changes Mar 11, 2026

View reviewed changes

gloriousCode requested changes Mar 11, 2026

View reviewed changes

websocket: move test hooks into helper file

9332623

gloriousCode approved these changes Mar 11, 2026

View reviewed changes

shazbert approved these changes Mar 11, 2026

View reviewed changes

thrasher- merged commit 654f4ed into thrasher-corp:master Mar 11, 2026
6 of 13 checks passed

		// Exchanges which don't support subscription conf; Can be removed when all exchanges support sub conf
		b.Websocket.GenerateSubs = func() (subscription.List, error) { return subscription.List{}, nil }

	// Exchanges which don't support subscription conf; Can be removed when all exchanges support sub conf
	b.Websocket.GenerateSubs = func() (subscription.List, error) { return subscription.List{}, nil }
	// Ensure no default subscriptions are required for tests, including exchanges without subscription configuration support.

		b.Websocket.GenerateSubs = func() (subscription.List, error) { return subscription.List{}, nil }
		b.Websocket.SetSubscriptionsNotRequired()

Conversation

thrasher- commented Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Description

What changed

Why

Type of change

How has this been tested

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shazbert left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

shazbert left a comment

Choose a reason for hiding this comment

Uh oh!

gloriousCode Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thrasher- commented Mar 11, 2026

Uh oh!

chatgpt-codex-connector bot commented Mar 11, 2026

Uh oh!

gloriousCode left a comment

Choose a reason for hiding this comment

Uh oh!

shazbert left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

thrasher- commented Mar 10, 2026 •

edited

Loading

gloriousCode Mar 11, 2026 •

edited

Loading